Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix mem::uninitialized UB warning #100

Merged
merged 1 commit into from
Dec 17, 2019
Merged

Fix mem::uninitialized UB warning #100

merged 1 commit into from
Dec 17, 2019

Conversation

SpaceManiac
Copy link
Contributor

Scary-looking warning fixable as far as I can tell with one-line patch

warning: use of deprecated item 'std::mem::uninitialized': use `mem::MaybeUninit` instead
    --> src/lib.rs:1138:29
     |
1138 |         self.map = unsafe { mem::uninitialized() };
     |                             ^^^^^^^^^^^^^^^^^^
     |
     = note: `#[warn(deprecated)]` on by default

warning: the type `std::collections::HashMap<KeyRef<K>, *mut Node<K, V>, S>` does not permit being left uninitialized
    --> src/lib.rs:1138:29
     |
1138 |         self.map = unsafe { mem::uninitialized() };
     |                             ^^^^^^^^^^^^^^^^^^^^
     |                             |
     |                             this code causes undefined behavior when executed
     |                             help: use `MaybeUninit<T>` instead
     |
     = note: `#[warn(invalid_value)]` on by default
note: std::ptr::NonNull<u8> must be non-null (in this struct field)

Gankra
Gankra approved these changes Dec 17, 2019
View reviewed changes
Copy link
Contributor

@Gankra Gankra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

woah yeah, weird whack code

@Gankra Gankra merged commit 4f681be into contain-rs:master Dec 17, 2019
@SpaceManiac SpaceManiac deleted the fix-ub-warnings branch January 13, 2020 03:25
@RalfJung
Copy link

RalfJung commented May 3, 2020

Any chance of a release with this fix? The crate is still fairly widely used, and with rust-lang/rust#71274 we are turning the UB here into a panic, which breaks the released version of this crate.

@RalfJung RalfJung mentioned this pull request May 3, 2020
Merged
@Gankra
Copy link
Contributor

Gankra commented May 6, 2020

Done.

@RalfJung
Copy link

RalfJung commented May 6, 2020

@Gankra looks like something went wrong? I cannot see a new version at https://crates.io/crates/linked-hash-map.

@Gankra
Copy link
Contributor

Gankra commented May 6, 2020

whoops sorry, fixed

@RalfJung
Copy link

RalfJung commented May 6, 2020

Awesome, thanks. :)

@RalfJung RalfJung mentioned this pull request May 14, 2020
Closed
@RalfJung RalfJung mentioned this pull request Jul 5, 2020
Closed
@Dylan-DPC-zz
Copy link

@Gankra is there any plan on yanking the older (unsound) versions? This crate has many reverse dependencies yanking the older ones would ensure that they obtain a sound version of this crate instead of bumping the minimum dependency to 0.5.3

@Bobo1239 Bobo1239 mentioned this pull request Oct 4, 2020
Closed
This was referenced Oct 5, 2020
Closed
Closed
@Xanewok
Copy link

Xanewok commented Nov 11, 2020

@Gankra a friendly reminder - it'd be great to yank the previous version as I've been bitten by this as well when using the toml_edit crate and had to scan the backtrace and dig a bit to find that I should bump this transitive dependency to 0.5.3.

Pretty please 🙇 (if not that's fine too ofc, just let us know 🙂)

@RalfJung
Copy link

FWIW, cargo audit should also tell you that this update is necessary.

Xanewok added a commit to paritytech/cargo-unleash that referenced this pull request Nov 13, 2020
@Xanewok
Fix TOML parse panic in newer Rust toolchains
232564b 
See contain-rs/linked-hash-map#100
quodlibetor added a commit to quodlibetor/materialize that referenced this pull request Nov 23, 2020
@quodlibetor
Update linked-hash-map
3064858 
There is a long-fixed issue[1] that we hit on this upgrade.

Honestly I'm surprised that dependabot didn't upgrade us on this.

[1]: contain-rs/linked-hash-map#100
Xanewok added a commit to paritytech/cargo-unleash that referenced this pull request Nov 24, 2020
@Xanewok
Fix TOML parse panic in newer Rust toolchains
2fa15ea 
See contain-rs/linked-hash-map#100
benesch pushed a commit to benesch/materialize that referenced this pull request Nov 24, 2020
@quodlibetor @benesch
Update linked-hash-map
41a8104 
There is a long-fixed issue[1] that we hit on this upgrade.

Honestly I'm surprised that dependabot didn't upgrade us on this.

[1]: contain-rs/linked-hash-map#100
@ash2x3zb9cy ash2x3zb9cy mentioned this pull request Nov 30, 2020
Closed
austinjones added a commit to austinjones/ttl_cache that referenced this pull request Dec 4, 2020
@austinjones
Require linked-hash-map v0.5.3 (or a newer patch version)
a2edfc0 
This commit requires linked-hash-map to include contain-rs/linked-hash-map#100

When linked-hash-map is on v0.5.2, ttl_cache can panic due to the UB checks implemented in Rust 1.48: rust-lang/rust#66151
@austinjones austinjones mentioned this pull request Dec 4, 2020
Closed
@teovoinea teovoinea mentioned this pull request Dec 30, 2020
Merged
@koba-e964 koba-e964 mentioned this pull request Jan 14, 2021
Merged
4 tasks
@dywedir dywedir mentioned this pull request Apr 24, 2021
Open
@dbaynard dbaynard mentioned this pull request Nov 23, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Gankra Gankra Gankra approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@SpaceManiac @RalfJung @Gankra @Dylan-DPC-zz @Xanewok